feat: Robot() factory + top-level lazy imports#86
Conversation
1e2d93b to
253c01a
Compare
yinsong1986
left a comment
There was a problem hiding this comment.
All review comments addressed. LGTM.
|
For all comments in this PR, we should examine common themes and include corrections for them in AGENTS.md so that future agent runs benefit from their lessons. |
Review Thread Triage (14 unresolved)Already fixed in code (4 threads from @yinsong1986) — need thread resolution:
New from @awsarron (10 threads, Apr 10) — need code work:
Additional blockers:
Recommendation: Wait for PR #84 to merge, then rebase and address @awsarron's 10 threads in one pass. Items 7 and 9 may need a design discussion before implementing. 🤖 Pipeline analysis by AI agent. Strands Agents. Feedback welcome! |
1. Rename factory.py → robot.py, robot.py → hardware_robot.py Eliminates two 'Robot' classes in different files. The factory function now lives where users expect: strands_robots.robot.Robot 2. Default mode='sim' instead of mode='auto' Using real hardware should be an explicit decision since it affects the physical world. Robot('so100') now always returns simulation. Use mode='real' to explicitly opt into hardware control. 3. Fix ThreadPoolExecutor leak in _async_utils.py Register atexit.shutdown(wait=False) to clean up the module-level executor on interpreter exit. 4. Remove redundant list_robots() wrapper Was a 1-line passthrough to registry.list_robots(). Now __init__.py points directly to strands_robots.registry.list_robots. 5. Use module names in dataset_recorder docstring 'robot.py' → 'strands_robots.hardware_robot', 'simulation.py' → 'strands_robots.simulation' 6. Make camera shape configurable in dataset_recorder Added camera_shapes parameter to _build_features() instead of hardcoding (3, 480, 640). Default preserved for backward compat. 7. Add mode validation — invalid mode raises ValueError 8. Update __init__.py lazy imports for renamed modules Tests: 230 passed, 10 skipped, 0 failures Lint: ruff check + ruff format clean
7c9cc11 to
3803e26
Compare
📋 Review Status SummaryHi @awsarron — this PR has 11/14 threads resolved. Here's a summary of the 3 remaining unresolved threads to help focus the re-review: Unresolved Thread 1: "Document all env vars in README"
➔ Cross-PR dependency: This is being addressed in PR #87 (docs rewrite), which includes comprehensive env var documentation. Suggest resolving this thread with a note that #87 covers it, or merging #87 first. Unresolved Thread 2: "Where is
|
…s-labs#86 - Add Environment Variables table to README documenting all 6 env vars used across the project (STRANDS_ROBOT_MODE, STRANDS_ASSETS_DIR, STRANDS_URDF_DIR, STRANDS_TRUST_REMOTE_CODE, GROOT_API_TOKEN, MUJOCO_GL) plus cache directory documentation - Add module-level docstring to dataset_recorder.py explaining why it lives at package root (shared by both hardware and simulation paths, avoids circular dependency) - Add docstring to load_lerobot_episode() documenting that it is consumed by simulation.mujoco.policy_runner for replay_episode
…ssets (#84) Simulation foundation layer for `strands-robots`. Pure Python, no MuJoCo dependency. Unblocks #85 (MuJoCo backend) and #86 (Robot factory). ## What's in **Simulation abstractions** (`strands_robots/simulation/`) - `models.py` — `SimWorld`, `SimRobot`, `SimObject`, `SimCamera`, `TrajectoryStep`, `SimStatus` dataclasses. Backend-agnostic: engine handles live in `_model`/`_data`, everything else in `_backend_state: dict`. - `base.py` — `SimEngine` ABC. 12 required abstract methods + 4 optional (raise `NotImplementedError`). Context-manager protocol. `__del__` logs cleanup errors at warning level. - `factory.py` — `create_simulation()` + `register_backend()` with duplicate/alias shadow protection (raises `ValueError`; `force=True` for intentional overrides). Descriptive `ImportError` when a built-in backend module isn't installed. - `model_registry.py` — URDF/MJCF resolution: user-registered → `STRANDS_ASSETS_DIR` → `~/.strands_robots/assets/` → CWD → `robot_descriptions` fallback. Resolves search paths at call time (no import-time `Path.cwd()` snapshot). - `__init__.py` — thin re-exports with lazy `__getattr__`. **Assets** (`strands_robots/assets/`) - `__init__.py` — thin exports only (repo convention). - `manager.py` — path resolution with `safe_join()` traversal protection. `_has_meshes()` uses `os.scandir` + early-exit, cached by `(path, mtime)`. Module-level guard for optional `[sim]` extra — no circular import with `download.py`. - `download.py` — all download logic (`robot_descriptions` → git clone fallback). `_shallow_clone()` enforces `_ALLOWED_CLONE_URL_RE` (HTTPS github.com only). `_copy_and_clean` filters ignored patterns at `copytree()` time so user files in the cache aren't clobbered. **Tools** (`strands_robots/tools/`) - `download_assets.py` — thin `@tool` wrapper (~78 lines) that delegates to `assets.download.download_robots()`. No duplicated logic. **Registry** (`strands_robots/registry/`) - `user_registry.py` — `register_robot()` / `unregister_robot()` persisted to `~/.strands_robots/user_robots.json`. Fails closed on missing asset dir. Warns on alias collisions at registration time. Docstring warns this must not be exposed as an agent `@tool` without `STRANDS_TRUST_REMOTE_CODE` gating (MJCF → MuJoCo plugin code-exec risk). - `loader.py` — merges user-local registry on top of package `robots.json`. Public `invalidate_cache()` API (no private imports from callers). - `robots.json` — 38 → 68 robots (adds aerial, expressive, mobile_manip categories). - `__init__.py` — re-exports `register_robot`, `unregister_robot`, `list_user_robots`, `invalidate_cache`. **Utils** (`strands_robots/utils.py`) - `get_base_dir()` reads `STRANDS_BASE_DIR` — decoupled from `STRANDS_ASSETS_DIR` so setting the assets path no longer drops `user_robots.json` into an unexpected parent. - `get_assets_dir()`, `resolve_asset_path()`, `safe_join()`, `get_search_paths()` — single source of truth; consumed by model_registry, user_registry, assets/manager. **Docs & packaging** - `README.md` — environment variables table (`STRANDS_BASE_DIR`, `STRANDS_ASSETS_DIR`, `GROOT_API_TOKEN`) + cache directory docs. - `AGENTS.md` — documents nested-asset-path convention (e.g. `xmls/asimov.xml` matching upstream layout) and the `auto_download` strategy invariant. - `pyproject.toml` — new `[sim]` extra (`robot_descriptions>=1.11.0,<2.0.0`); included in `[all]`. ## Design decisions **SimEngine ABC contract.** 12 required methods every physics engine must implement; 4 optional (`load_scene`, `run_policy`, `randomize`, `get_contacts`) raise `NotImplementedError` so unimplemented features are explicit during development. `get_observation`/`send_action` are deliberately facade methods bridging Sim ↔ Policy — the agent tool sees a single interface without needing to know the Robot vs Sim split. **Asset resolution order.** Customer assets always win over defaults: `STRANDS_ASSETS_DIR` → `~/.strands_robots/assets/` → `CWD/assets/` → `robot_descriptions` fallback. Single env var for the asset tree (`STRANDS_ASSETS_DIR`); separate `STRANDS_BASE_DIR` for the base dir that holds `user_robots.json`. **Backend registration.** `register_backend()` rejects duplicates by default and blocks shadowing of built-in aliases (`mj`, `mjc`, `mjx`) unless `force=True`. Alias conflicts caught at both the `name` and `aliases` parameters. **Security.** - `safe_join()` applied everywhere registry values flow into filesystem paths (manager + download + user registry). - `_shallow_clone()` URL regex rejects `ssh://`, `git://`, `file://`, non-github hosts. - `register_robot()` is library-only; not surfaced as `@tool`. Docstring spells out the MJCF-plugin exec risk. ## Testing - 338 unit tests pass, 6 skipped, 0 failures - `ruff check` + `ruff format --check`: clean (57 files) - `mypy`: 0 issues in 57 source files - New test files: - `tests/test_simulation_foundation.py` — ABC contracts, factory round-trip, context-manager cleanup - `tests/test_simulation_factory.py` — duplicate rejection, alias shadowing, missing-backend ImportError - `tests/test_user_registry.py` — register/unregister, persistence, validation, path traversal; asserts `STRANDS_ASSETS_DIR` does NOT move the base dir / registry - `tests/test_registry_integrity.py` — auto-download invariant, alias uniqueness, canonical-shadow protection, lerobot_type presence on hardware-only robots ## Review history | Reviewer | Status | Threads | |-------------------|---------------------------------------|-----------------| | @yinsong1986 | APPROVED | 3/3 resolved | | @awsarron | CHANGES_REQUESTED → all addressed | 50/50 addressed | | @max-rattray-aws | COMMENTED → all addressed | 3/3 resolved | Closes #84. --------- Co-authored-by: cagataycali <cagataycali@icloud.com> Co-authored-by: strands-agent <217235299+strands-agent@users.noreply.github.com>
1. Rename factory.py → robot.py, robot.py → hardware_robot.py Eliminates two 'Robot' classes in different files. The factory function now lives where users expect: strands_robots.robot.Robot 2. Default mode='sim' instead of mode='auto' Using real hardware should be an explicit decision since it affects the physical world. Robot('so100') now always returns simulation. Use mode='real' to explicitly opt into hardware control. 3. Fix ThreadPoolExecutor leak in _async_utils.py Register atexit.shutdown(wait=False) to clean up the module-level executor on interpreter exit. 4. Remove redundant list_robots() wrapper Was a 1-line passthrough to registry.list_robots(). Now __init__.py points directly to strands_robots.registry.list_robots. 5. Use module names in dataset_recorder docstring 'robot.py' → 'strands_robots.hardware_robot', 'simulation.py' → 'strands_robots.simulation' 6. Make camera shape configurable in dataset_recorder Added camera_shapes parameter to _build_features() instead of hardcoding (3, 480, 640). Default preserved for backward compat. 7. Add mode validation — invalid mode raises ValueError 8. Update __init__.py lazy imports for renamed modules Tests: 230 passed, 10 skipped, 0 failures Lint: ruff check + ruff format clean
…s-labs#86 - Add Environment Variables table to README documenting all 6 env vars used across the project (STRANDS_ROBOT_MODE, STRANDS_ASSETS_DIR, STRANDS_URDF_DIR, STRANDS_TRUST_REMOTE_CODE, GROOT_API_TOKEN, MUJOCO_GL) plus cache directory documentation - Add module-level docstring to dataset_recorder.py explaining why it lives at package root (shared by both hardware and simulation paths, avoids circular dependency) - Add docstring to load_lerobot_episode() documenting that it is consumed by simulation.mujoco.policy_runner for replay_episode
449aa05 to
0917c24
Compare
|
Rebased onto the updated #85 (which itself is now rebased on main). Stack status:
What changed:
Conflict resolution:
Quality gate:
🤖 AI agent response. Strands Agents. Feedback welcome! |
Rebased on main (post-PR strands-labs#85 merge). Changes: - New strands_robots/robot.py: Robot() factory function - Default mode='sim' (safe — never sends commands to hardware) - mode='real' for explicit hardware opt-in - mode='auto' probes USB for servo controllers - Rename old robot.py → hardware_robot.py (HardwareRobot class) - Updated __init__.py: lazy imports for Simulation, SimWorld, SimRobot, SimObject, SimCamera, list_robots, create_simulation, etc. - Auto-configure MUJOCO_GL at import time (before mujoco locks backend) - Registry updates: minor em-dash formatting consistency - Tests: test_robot_factory.py, test_registry.py, test_registry_integrity.py, test_user_registry.py All review threads (14/14) previously resolved. Addresses feedback from @awsarron and @yinsong1986.
bb959e4 to
bf277f5
Compare
All 14 review items addressed. PR rebased on main (post-#85 merge). CI passing.
✅ Rebased & CI Green
Key changes from the rebase:
Ready for re-review when convenient. 🙏 🤖 AI agent. Strands Agents. Feedback welcome! |
|
Rebased on Changes from the rebase:
Awaiting re-approval (force push invalidated the previous approval). 🤖 AI agent response. Strands Agents. Feedback welcome! |
Tests in tests/simulation/mujoco/ and tests/simulation/ that import or instantiate MuJoCo simulations now properly skip when mujoco is not installed, instead of raising ImportError during collection. Files fixed: - test_agenttool_contract.py - test_backend.py (TestEnsureMujoco class only) - test_recording_backends.py - test_factory.py (TestCreateSimulation class only) - test_policy_runner_behaviour.py
yinsong1986
left a comment
There was a problem hiding this comment.
Review summary — direction is right (sim-by-default + factory split is the right safety call), tests are thorough, but a handful of items would be good to address before merge. Inline comments below; full prose review at the top.
Higher-priority:
- Return type
Anydefeats type checking; suggestSimulation | HardwareRobot(or@overload). _dispatch_action("create_world", {})ignores its return — failures here surface as misleading errors on the next call.cameras=is silently dropped in sim mode — either reject or forward.data_config=canonicalcollapses two distinct concepts (e.g.so100vsso100_dualcam).- PR description's file table doesn't match the diff (no
factory.py/test_factory.pyfiles added; tests live inrobot.py/test_robot_factory.py).
Smaller:
6. Eager _configure_gl_backend() import on every import strands_robots.
7. Reaching into sim._dispatch_action(...) (private) couples the factory to AgentTool internals.
8. os.environ[...] direct mutation in tests should use monkeypatch.setenv for crash-safety + suite consistency.
9. Emoji regression in hardware_robot.py vs the prior robot.py — confirm intentional.
10. Bad STRANDS_ROBOT_MODE values silently fall through to sim; log a warning.
11. backend= ignored when mode="real".
12. No test exercises mode="real" even with mocking; no test for the USB-found-hardware branch in _auto_detect_mode.
Review fixes (yinsong1986 2026-05-13):
1. Return type: added @typing.overload for mode='sim'→Simulation,
mode='real'→HardwareRobot, mode='auto'→union. Added # noqa: N802
for uppercase factory name.
2. cameras= rejected in sim mode: raises ValueError with guidance
to use sim._dispatch_action('add_camera', {...}) after creation.
3. create_world error handling: result is now checked; on error,
sim.destroy() fires and RuntimeError raised with context.
4. data_config decoupled from robot name: new data_config parameter
(defaults to canonical name). Robot('so100', data_config='so100_dualcam')
now works.
5. _dispatch_action coupling acknowledged: kept for now (promoting to
public or adding shorthands is follow-up scope per reviewer).
6. backend= ignored in mode='real': logs debug message instead of
silently dropping.
7. Unrecognized STRANDS_ROBOT_MODE: logs warning and falls through to
sim (previously silent). Added test.
8. __init__.py eager import guarded: importlib.util.find_spec('mujoco')
skips the GL backend config when mujoco isn't installed.
9. Tests use monkeypatch: replaced direct os.environ mutation with
monkeypatch.setenv (no leak risk).
10. Emoji regression in hardware_robot.py: all emojis stripped from
logger calls and result-text strings.
yinsong1986
left a comment
There was a problem hiding this comment.
Second-pass review (commit 376376b — "address 10 review comments")
Thanks for the quick turnaround. Most items are well-handled; flagging what's genuinely fixed vs. what still needs another pass.
✅ Resolved cleanly
- Type signature — three
@overloads +Simulation | HardwareRobotreturn +# noqa: N802comment. Exactly what was asked. create_worldreturn value — now checked, withsim.destroy()cleanup +RuntimeError. Mirrorsadd_robotpattern.cameras=in sim mode — rejected with a clearValueErrorpointing users at_dispatch_action("add_camera", ...). Plus a new testtest_cameras_rejected_in_sim_mode.data_configseparation — newdata_config: str | None = Nonekwarg, defaults tocanonical.Robot("so100", data_config="so100_dualcam")now works. Docstring +Raisesupdated.- Bad
STRANDS_ROBOT_MODEvalue warning — added atrobot.py:64, plus a new testtest_unrecognized_env_value_falls_through. backend=in real mode — debug log explaining it's ignored. Reasonable choice.- Eager GL backend import guard —
importlib.util.find_spec("mujoco") is not Noneskip added. monkeypatch.setenv— all four env-var tests migrated.import osremoved. Cleaner and crash-safe.
⚠️ Partially resolved — needs one more pass
- Emoji cleanup left orphan combining marks. Several lines in
hardware_robot.pynow contain stray\xef\xb8\x8f(variation selector) bytes that used to follow⏱️— visible as a leading space in the rendered text (" Duration: ..."). See inline comment. - Emoji cleanup wasn't complete. A handful of emojis survived in result strings (
💡,🔄). If the rule is "no emoji," finish the sweep. If "some are fine," pick a rule and apply consistently. Inline comment lists the survivors.
❌ Still outstanding from round 1
- PR description still wrong. Table still references
strands_robots/factory.py | 199andtests/test_factory.py | 148— neither file exists. Real files:strands_robots/robot.py(now ~280 lines after this commit),tests/test_robot_factory.py(~196 lines). Trivial to fix; helps future readers/auditors. sim._dispatch_action(...)still private. Not addressed — factory still depends on a private method, and the docstring now even tells users to call it themselves (sim._dispatch_action('add_camera', {...})). This is now codified in user-facing docs, which makes it harder to take back. Inline comment.- No
mode="real"test. Still no smoke test for the real-hardware path (even withmock.patch). - No USB-found-hardware branch test for
_auto_detect_mode. The newtest_unrecognized_env_value_falls_throughis good but doesn't reach theserial.tools.list_ports.comports()branch.
Verdict
Close. Items 1+2 (emoji hygiene) and item 3 (PR description) are quick. Items 4+5+6 are reasonable as follow-ups. Happy to approve once the orphan-mark text strings are fixed and the description matches the diff.
…real-mode tests Addresses round-2 review items: - Remove orphan U+FE0F variation selectors in hardware_robot.py (stray combining marks left after emoji removal in previous commit) - Remove remaining emojis (💡, 🔄) in hardware_robot.py result strings - Add TestRobotRealMode: smoke test for mode='real' path (mocked) - Add TestAutoDetectUSB: tests for USB servo detection branch, bluetooth exclusion, pyserial ImportError fallback, and no-hardware-support path
|
Thank you @yinsong1986 for the thorough two-round review — this is exactly the kind of detailed feedback that makes the codebase better. Summary of what I'm tracking:✅ Confirmed resolved (8 items) — great, no action needed.🔧 Will fix in next push (quick):
📝 Noted for follow-up (agreed these are separable):
Timing note: I'm currently waiting for @awsarron's review on PR #145 (my active PR). Once that resolves, I'll push the fixes above to #86 immediately. The fixes are trivial and well-scoped — shouldn't take more than one commit. Appreciate the clear verdict — makes it easy to know exactly what's needed! 🤖 AI agent response. Strands Agents. Feedback welcome! |
TL;DR
Add
Robot()factory function that auto-detects sim vs real mode, plus lazy imports in__init__.pyfor all simulation types.What changed
strands_robots/robot.pyRobot()factory +_auto_detect_mode()+@overloadtype signaturesstrands_robots/__init__.pystrands_robots/hardware_robot.pyrobot.py→hardware_robot.py, class staysRobotinternallystrands_robots/registry/tests/test_robot_factory.pyUsage
Auto-detect logic
STRANDS_ROBOT_MODEenv var (explicit override)"sim"(safest — never accidentally send commands to hardware)Type safety
Three
@overloadsignatures so IDEs resolve:Robot("so100", mode="sim")→SimulationRobot("so100", mode="real")→HardwareRobotRobot("so100", mode="auto")→Simulation | HardwareRobotKnown follow-ups (not blocking)
sim._dispatch_action(...)is private — promote to public or add thin wrappers (tracked separately)cameras=forwarding to sim mode onceSimCamera.parent_bodylandsTesting
Part 5 of 6 in the MuJoCo simulation PR decomposition